NE-2032: Refactor GatewayAPI DNS e2e #1226
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@grzpiotrowski: This pull request references NE-2032 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Unrelated, I think. /retest |
|
/assign |
|
/refresh |
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
Some minor remarks. There was a remark in the previous PR about the cleanup function. Do you think you can consider it too?
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Does checkDNSRecordExpectation even return an error? I can only see nils returned. If no error can be returned from checkDNSRecordExpectation then it has to return only 1 value (bool).
There was a problem hiding this comment.
I would like to see the messages that are logged here, be returned to the caller in this err variable so the caller can figure out what to do with the message, instead of logging it here. Some messages work great for polling, e.g. "DNSRecord %s found but not yet published, retrying...", but don't make sense if you wanted to reuse this function in the future for a one-time call instead of repeated polling.
So let the caller decide what to do with the values returned.
There was a problem hiding this comment.
Yes, I overlooked this. Indeed, checkDNSRecordExpectation can be a simple function, not a test helper. The logging is better to be done upper the call stack along with the other logs on the same level.
Even a single error return value can be enough:
if err := checkDNSRecordExpectation(dnsRecords, exp, shouldBePresent); err != nil {
t.Log(err)
return false, nil
}
| if err := kclient.List(ctx, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil { | ||
| return false, fmt.Errorf("failed to list DNSRecords: %v", err) |
There was a problem hiding this comment.
In most of the polling cases we log the error and retry the API operator. Returning an error would break the polling loop.
| if err := kclient.List(ctx, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil { | |
| return false, fmt.Errorf("failed to list DNSRecords: %v", err) | |
| if err := kclient.List(ctx, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil { | |
| t.Logf("Failed to list DNSRecords: %v", err) | |
| return false, nil |
| return false, nil | ||
| } | ||
| for exp, shouldBePresent := range expectations { | ||
| expectationMet, err := checkDNSRecordExpectation(t, *dnsRecords, exp, shouldBePresent) |
There was a problem hiding this comment.
I think it's fine to pass by pointer to gain some CPU cycles on copying of all DNSRecords of the cluster.
| expectationMet, err := checkDNSRecordExpectation(t, *dnsRecords, exp, shouldBePresent) | |
| expectationMet, err := checkDNSRecordExpectation(t, dnsRecords, exp, shouldBePresent) |
Unfortunately, go doesn't have pointers to const (hi, C++!) to benefit from the pointer parameter and ensure no modification on the pointed object. But it's quite common to pass by pointer even in read-only cases.
| if haveExpectNotPresent { | ||
| t.Logf("Continuing polling to ensure non-expected DNSRecords do not exist...") | ||
| if !shouldBePresent { | ||
| t.Logf("DNSRecord %s found but expected it absent.", exp.dnsName) |
There was a problem hiding this comment.
| t.Logf("DNSRecord %s found but expected it absent.", exp.dnsName) | |
| t.Logf("DNSRecord %q found but expected to be absent; retrying...", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
| t.Logf("DNSRecord %s found but not yet published, retrying...", exp.dnsName) | ||
| return false, nil | ||
| } | ||
| t.Logf("DNSRecord %s found & published as expected", exp.dnsName) |
There was a problem hiding this comment.
| t.Logf("DNSRecord %s found & published as expected", exp.dnsName) | |
| t.Logf("DNSRecord %q found and published as expected", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
| return false, nil | ||
| } | ||
| return nil | ||
| t.Logf("DNSRecord %s correctly absent", exp.dnsName) |
There was a problem hiding this comment.
| t.Logf("DNSRecord %s correctly absent", exp.dnsName) | |
| t.Logf("DNSRecord %q absent as expected", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
| } | ||
|
|
||
| // isPublished returns true if the DNSRecord.Status.Zones contains a Published condition = True. | ||
| func isPublished(rec v1.DNSRecord) bool { |
There was a problem hiding this comment.
To be consistent in the naming with dnsRecordMatches:
| func isPublished(rec v1.DNSRecord) bool { | |
| func dnsRecordPublished(rec v1.DNSRecord) bool { |
| return !haveExpectNotPresent, nil | ||
| }) | ||
| if !isPublished(rec) { | ||
| t.Logf("DNSRecord %s found but not yet published, retrying...", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
candita
left a comment
candita
left a comment
There was a problem hiding this comment.
Looks much better, thanks. Just a few nits.
| t.Logf("DNSRecord %s found but not yet published, retrying...", exp.dnsName) | ||
| return false, nil | ||
| } | ||
| t.Logf("DNSRecord %s found & published as expected", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
| if !expectationsMet { | ||
| return fmt.Errorf("failed to observe expected DNSRecords: %v", err) | ||
| if shouldBePresent { | ||
| t.Logf("DNSRecord %s not found, retrying...", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
| return false, nil | ||
| } | ||
| return nil | ||
| t.Logf("DNSRecord %s correctly absent", exp.dnsName) |
There was a problem hiding this comment.
See https://github.com/openshift/cluster-ingress-operator/pull/1226/files#r2093564183 on returning this error instead of logging it.
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
@grzpiotrowski: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
This PR refactors the Gateway API DNS helper functions to improve maintainability and aid in further enhancing the tests, refactoring particularly around the
cluster-ingress-operator/test/e2e/util_gatewayapi_test.go: assertExpectedDNSRecords function.
This is a follow up to comments in PR #1213 which added Gateway API DNS e2e tests.